Power spectral density curves settings improvement #844
Conversation
## Walkthrough
A numeric input for segment length was added to the analyser's Power Spectral Density (PSD) controls in the UI. The input is integrated into the UI logic, PSD calculation module, and CSS. The PSD calculation now uses this user-defined segment length, and related event handling and state management were updated accordingly. Additionally, imported curve management was modularized, legend positioning controls were added, and export logic was refined.
## Changes
| File(s) | Change Summary |
|--------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------|
| index.html | Added numeric input and label for PSD segment length and legend position/size inputs in analyser settings and user settings dialog. |
| src/css/main.css | Added and updated styles for the new PSD segment length input and its label for consistent appearance and positioning. |
| src/graph_spectrum.js | Integrated segment length input into UI logic, event handling, and PSD data reloading; updated state management and visibility; simplified CSV import; added methods to remove imported spectrums and get export filename. |
| src/graph_spectrum_calc.js | Added segment length property and setter; updated PSD calculation to use user-defined segment length; adjusted overlap logic; renamed return fields. |
| public/js/webworkers/spectrum-export-worker.js | Modified FFT data export to include all data points with new CSV headers. |
| src/graph_imported_curves.js | Added new module managing imported curves from CSV files with import, count, and removal functionality. |
| src/graph_spectrum_plot.js | Refactored imported spectrum handling to use ImportedCurves instances; updated drawing logic for frequency and PSD graphs; added legend drawing with user-configured layout; replaced frequency constants; added import/remove curve methods. |
| src/main.js | Updated exportSpectrumToCsv to use dynamic filename from analyser; renamed method to remove imported spectrums; updated button handlers accordingly. |
| src/user_settings_dialog.js | Added new analyser_legend settings group with left, top, and width properties; updated UI-to-settings and settings-to-UI conversions. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant UI (index.html)
participant GraphSpectrum
participant GraphSpectrumCalc
User->>UI (index.html): Change segment length input
UI (index.html)->>GraphSpectrum: Input event triggers handler
GraphSpectrum->>GraphSpectrumCalc: setPointsPerSegmentPSD(newLength)
GraphSpectrum->>GraphSpectrumCalc: dataLoadPSD()
GraphSpectrumCalc-->>GraphSpectrum: Returns PSD data
GraphSpectrum->>UI (index.html): Update plot with new PSD dataPossibly related PRs
Suggested reviewers
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
index.html (1)
496-515: Wire up PSD segment-length persistence and restoreThe segment-length control is correctly clamped to powers-of-two and toggled based on spectrum type, but it isn’t saved or initialized from user settings. Without this, users will lose their chosen segment length on refresh.
• In src/graph_spectrum.js, around line 302–304 (initialization):
– LoaduserSettings.psdSegmentLength(or fallback toDEFAULT_PSD_SEGMENT_LENGTH) before callingGraphSpectrumCalc.setPointsPerSegmentPSD(...)and set the input’s.val(...).• In the
.on("input", …)handler at line 304–318:
– After updatingsegmentLenghtPSD, callsaveOneUserSetting("psdSegmentLength", segmentLenghtPSD).Suggested diff snippet:
--- a/src/graph_spectrum.js +++ b/src/graph_spectrum.js @@ 300,7 +300,15 @@ - let segmentLenghtPSD = DEFAULT_PSD_SEGMENT_LENGTH; - GraphSpectrumCalc.setPointsPerSegmentPSD(segmentLenghtPSD); - analyserSegmentLengthPSD + // Restore saved PSD segment length + let segmentLenghtPSD = + userSettings.psdSegmentLength ?? DEFAULT_PSD_SEGMENT_LENGTH; + analyserSegmentLengthPSD.val(segmentLenghtPSD); + GraphSpectrumCalc.setPointsPerSegmentPSD(segmentLenghtPSD); + saveOneUserSetting("psdSegmentLength", segmentLenghtPSD); + analyserSegmentLengthPSD @@ 307,6 +315,7 @@ $(this).val(segmentLenghtPSD); // Recalculate PSD with updated samples per segment count GraphSpectrumCalc.setPointsPerSegmentPSD(segmentLenghtPSD); + saveOneUserSetting("psdSegmentLength", segmentLenghtPSD); dataLoad(); GraphSpectrumPlot.setData(fftData, userSettings.spectrumType); that.refresh();This ensures the control’s value is persisted and restored.
🧹 Nitpick comments (2)
index.html (2)
496-497: Associate the new input with its label for accessibilityThe new
<input>lacks an explicit association with its descriptive label (<label id="analyserSegmentLengthPSDLabel">). Adding afor="analyserSegmentLengthPSD"attribute on the label (and keeping the input’sid) will let screen–reader and form–navigation tools recognise the relationship.-<label id="analyserSegmentLengthPSDLabel" name="analyserSegmentLengthPSDLabel" class="onlyFullScreen" > +<label for="analyserSegmentLengthPSD" + id="analyserSegmentLengthPSDLabel" + name="analyserSegmentLengthPSDLabel" + class="onlyFullScreen">
513-515: Minor wording: “Segment length” (singular) reads betterUI copy elsewhere uses singular nouns (“Limit dBm”, “Max dBm”). Consider changing the caption to “Segment length” for consistency and readability.
- Segments length + Segment length
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
index.html(2 hunks)src/css/main.css(2 hunks)src/graph_spectrum.js(8 hunks)src/graph_spectrum_calc.js(3 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#833
File: src/graph_spectrum.js:0-0
Timestamp: 2025-05-26T16:18:25.863Z
Learning: In src/graph_spectrum_calc.js, the user created dedicated PSD functions `dataLoadPowerSpectralDensityVsThrottle()` and `dataLoadPowerSpectralDensityVsRpm()` instead of using boolean parameters, following a consistent naming pattern for spectrum data loading methods.
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#0
File: :0-0
Timestamp: 2025-06-29T18:40:50.980Z
Learning: In betaflight/blackbox-log-viewer, when implementing PSD heatmap settings persistence in src/graph_spectrum.js, the initialization sequence is critical - user settings must be initialized and applied to GraphSpectrumPlot before attaching input event handlers to prevent spurious refresh() calls that can cause spectrum rendering errors when disabled.
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#827
File: src/graph_spectrum.js:200-207
Timestamp: 2025-05-10T19:47:41.430Z
Learning: The vertical zoom slider recalculation in the spectrum analyzer is designed to work only for POWER_SPECTRAL_DENSITY spectrum type, not for PSD_VS_THROTTLE or PSD_VS_RPM types.
Learnt from: Quick-Flash
PR: betaflight/blackbox-log-viewer#0
File: :0-0
Timestamp: 2025-05-13T17:48:20.477Z
Learning: When using FFT (Fast Fourier Transform), enforcing power-of-2 input lengths improves computational efficiency and numerical stability but can result in discarding up to almost 50% of the flight data in worst-case scenarios.
index.html (3)
undefined
<retrieved_learning>
Learnt from: demvlad
PR: #833
File: src/graph_spectrum.js:0-0
Timestamp: 2025-05-26T16:18:25.863Z
Learning: In src/graph_spectrum_calc.js, the user created dedicated PSD functions dataLoadPowerSpectralDensityVsThrottle() and dataLoadPowerSpectralDensityVsRpm() instead of using boolean parameters, following a consistent naming pattern for spectrum data loading methods.
</retrieved_learning>
<retrieved_learning>
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#0
File: :0-0
Timestamp: 2025-06-29T18:40:50.980Z
Learning: In betaflight/blackbox-log-viewer, when implementing PSD heatmap settings persistence in src/graph_spectrum.js, the initialization sequence is critical - user settings must be initialized and applied to GraphSpectrumPlot before attaching input event handlers to prevent spurious refresh() calls that can cause spectrum rendering errors when disabled.
</retrieved_learning>
<retrieved_learning>
Learnt from: demvlad
PR: #827
File: src/graph_spectrum.js:200-207
Timestamp: 2025-05-10T19:47:41.430Z
Learning: The vertical zoom slider recalculation in the spectrum analyzer is designed to work only for POWER_SPECTRAL_DENSITY spectrum type, not for PSD_VS_THROTTLE or PSD_VS_RPM types.
</retrieved_learning>
src/css/main.css (3)
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#833
File: src/graph_spectrum.js:0-0
Timestamp: 2025-05-26T16:18:25.863Z
Learning: In src/graph_spectrum_calc.js, the user created dedicated PSD functions `dataLoadPowerSpectralDensityVsThrottle()` and `dataLoadPowerSpectralDensityVsRpm()` instead of using boolean parameters, following a consistent naming pattern for spectrum data loading methods.
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#827
File: src/graph_spectrum.js:200-207
Timestamp: 2025-05-10T19:47:41.430Z
Learning: The vertical zoom slider recalculation in the spectrum analyzer is designed to work only for POWER_SPECTRAL_DENSITY spectrum type, not for PSD_VS_THROTTLE or PSD_VS_RPM types.
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#0
File: :0-0
Timestamp: 2025-06-29T18:40:50.980Z
Learning: In betaflight/blackbox-log-viewer, when implementing PSD heatmap settings persistence in src/graph_spectrum.js, the initialization sequence is critical - user settings must be initialized and applied to GraphSpectrumPlot before attaching input event handlers to prevent spurious refresh() calls that can cause spectrum rendering errors when disabled.
src/graph_spectrum.js (4)
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#833
File: src/graph_spectrum.js:0-0
Timestamp: 2025-05-26T16:18:25.863Z
Learning: In src/graph_spectrum_calc.js, the user created dedicated PSD functions `dataLoadPowerSpectralDensityVsThrottle()` and `dataLoadPowerSpectralDensityVsRpm()` instead of using boolean parameters, following a consistent naming pattern for spectrum data loading methods.
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#0
File: :0-0
Timestamp: 2025-06-29T18:40:50.980Z
Learning: In betaflight/blackbox-log-viewer, when implementing PSD heatmap settings persistence in src/graph_spectrum.js, the initialization sequence is critical - user settings must be initialized and applied to GraphSpectrumPlot before attaching input event handlers to prevent spurious refresh() calls that can cause spectrum rendering errors when disabled.
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#827
File: src/graph_spectrum.js:200-207
Timestamp: 2025-05-10T19:47:41.430Z
Learning: The vertical zoom slider recalculation in the spectrum analyzer is designed to work only for POWER_SPECTRAL_DENSITY spectrum type, not for PSD_VS_THROTTLE or PSD_VS_RPM types.
Learnt from: Quick-Flash
PR: betaflight/blackbox-log-viewer#0
File: :0-0
Timestamp: 2025-05-13T17:48:20.477Z
Learning: When using FFT (Fast Fourier Transform), enforcing power-of-2 input lengths improves computational efficiency and numerical stability but can result in discarding up to almost 50% of the flight data in worst-case scenarios.
src/graph_spectrum_calc.js (5)
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#833
File: src/graph_spectrum.js:0-0
Timestamp: 2025-05-26T16:18:25.863Z
Learning: In src/graph_spectrum_calc.js, the user created dedicated PSD functions `dataLoadPowerSpectralDensityVsThrottle()` and `dataLoadPowerSpectralDensityVsRpm()` instead of using boolean parameters, following a consistent naming pattern for spectrum data loading methods.
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#0
File: :0-0
Timestamp: 2025-06-29T18:40:50.980Z
Learning: In betaflight/blackbox-log-viewer, when implementing PSD heatmap settings persistence in src/graph_spectrum.js, the initialization sequence is critical - user settings must be initialized and applied to GraphSpectrumPlot before attaching input event handlers to prevent spurious refresh() calls that can cause spectrum rendering errors when disabled.
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#827
File: src/graph_spectrum.js:200-207
Timestamp: 2025-05-10T19:47:41.430Z
Learning: The vertical zoom slider recalculation in the spectrum analyzer is designed to work only for POWER_SPECTRAL_DENSITY spectrum type, not for PSD_VS_THROTTLE or PSD_VS_RPM types.
Learnt from: Quick-Flash
PR: betaflight/blackbox-log-viewer#0
File: :0-0
Timestamp: 2025-05-13T17:48:20.477Z
Learning: When using FFT (Fast Fourier Transform), enforcing power-of-2 input lengths improves computational efficiency and numerical stability but can result in discarding up to almost 50% of the flight data in worst-case scenarios.
Learnt from: nerdCopter
PR: betaflight/blackbox-log-viewer#829
File: src/graph_spectrum_calc.js:332-335
Timestamp: 2025-05-12T19:49:35.667Z
Learning: In the Blackbox Log Viewer, the `_getFlightSamplesFreq()` function should return a sliced array `samples.slice(0, samplesCount)` rather than the full preallocated array, as code that consumes this function expects the array length to match the count of valid samples.
🔇 Additional comments (12)
src/css/main.css (2)
693-695: LGTM: Consistent spin button styling applied.The CSS rule correctly extends the existing webkit spin button appearance overrides to include the new segment length input, maintaining visual consistency with other PSD inputs.
749-763: LGTM: Proper positioning and styling for new input controls.The CSS positioning and styling for the segment length input and label follow the established pattern of other analyser controls, ensuring consistent appearance and layout.
src/graph_spectrum_calc.js (3)
38-38: LGTM: Appropriate property addition.The new private property
_pointsPerSegmentPSDproperly stores the user-defined segment length for PSD calculations.
116-118: LGTM: Clean setter implementation.The setter method provides a clean interface for updating the PSD segment length from the UI layer.
122-129: Excellent refactoring of PSD calculation logic.The changes successfully decouple PSD segment length from zoom controls, allowing explicit user control. The logic appropriately handles edge cases where the requested segment length exceeds available samples, and the returned
maximalSegmentsLengthenables proper UI validation.Note: The overlap calculation has changed from 50% to 75% (
pointsPerSegment * 3 / 4), which will provide better frequency resolution at the cost of some computational overhead.Also applies to: 142-142
src/graph_spectrum.js (7)
20-21: LGTM: Appropriate default value.The default segment length of 512 is a good power-of-2 value that balances frequency resolution with computational efficiency.
39-39: LGTM: Consistent DOM element reference.The new element reference follows the established naming pattern for other analyser controls.
122-127: LGTM: Proper positioning integration.The positioning logic for the new segment length input and label is properly integrated with the existing resize functionality.
158-158: Excellent dynamic validation integration.Setting the max property based on
maximalSegmentsLengthfrom the FFT data ensures the UI prevents invalid segment length values that exceed available data.
330-336: LGTM: Effective input restriction.The keydown event handler properly prevents arbitrary input, ensuring only power-of-2 values can be set through the UI interaction logic.
362-372: Excellent UI state management.The spectrum type change handling properly shows/hides the segment length control only for the POWER_SPECTRAL_DENSITY type, maintaining clean UI organization and following the established pattern for other spectrum-specific controls.
Also applies to: 397-400
253-253: LGTM: Simplified input handling.Removing the conditional check before setting the low-level PSD value simplifies the logic appropriately.
|
@mituritsyn, please, could you have look at this improvement |
|
functionally it is good. Small visual notice - the signature looks a bit out of place, perhaps it would be bettre to slightly adjust the alignment.
It is appropriately aligned on the frequency plot. |
This was common issue for the all filters at the PSD curve chart only. Resolved. |
…_FREQUENCY in graph_spectrum_plot.js
Co-authored-by: Mark Haslinghuis <mark@numloq.nl>
Co-authored-by: Mark Haslinghuis <mark@numloq.nl>
Co-authored-by: Mark Haslinghuis <mark@numloq.nl>
Co-authored-by: Mark Haslinghuis <mark@numloq.nl>
…or PSD segment length values input
…rm with shows logic
|
I think, maybe will better show power at 2 value instead of the full segments length |
|
|
Preview URL: https://5513d867.betaflight-blackbox.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/user_settings_dialog.js (1)
663-671: Consider adding input validation for percentage values.While the settings loading logic is correct, consider adding validation to ensure the percentage values are within acceptable ranges (0-100).
+// Add validation helper function +function validatePercentage(value, min = 0, max = 100) { + const num = parseInt(value); + return Math.max(min, Math.min(max, isNaN(num) ? 0 : num)); +} $('.analyser-settings input[name="analyser-legend-top"]').val( - parseInt(currentSettings.analyser_legend.top), + validatePercentage(currentSettings.analyser_legend.top), ); $('.analyser-settings input[name="analyser-legend-left"]').val( - parseInt(currentSettings.analyser_legend.left), + validatePercentage(currentSettings.analyser_legend.left), ); $('.analyser-settings input[name="analyser-legend-width"]').val( - parseInt(currentSettings.analyser_legend.width), + validatePercentage(currentSettings.analyser_legend.width), );src/graph_imported_curves.js (1)
37-49: Consider performance optimization for large datasets.The current implementation updates min/max values for every point, which could be inefficient for large datasets. Consider batch processing or using more efficient algorithms.
+// Process min/max in batches for better performance +const xs = curvesData.map(point => point.x); +const ys = curvesData.map(point => point.y); +_that.minX = Math.min(_that.minX, Math.min(...xs)); +_that.maxX = Math.max(_that.maxX, Math.max(...xs)); +_that.minY = Math.min(_that.minY, Math.min(...ys)); +_that.maxY = Math.max(_that.maxY, Math.max(...ys));src/graph_spectrum_plot.js (2)
60-68: Consider supporting more than 5 imported curves.The
curvesColorsarray only contains 5 colors, which limits the number of distinguishable imported curves. If users import more than 5 curves, the colors will be undefined.Consider implementing a color generation function or expanding the color palette:
- curvesColors : [ - "Blue", - "Purple", - "DeepPink", - "DarkCyan", - "Chocolate", - ], + curvesColors : [ + "Blue", + "Purple", + "DeepPink", + "DarkCyan", + "Chocolate", + "Orange", + "Green", + "Red", + "Gold", + "Lime", + ], + getCurveColor: function(index) { + // Cycle through colors if more curves than colors + return this.curvesColors[index % this.curvesColors.length]; + },
274-274: Consider making the filter window size configurable.The filter window size has been increased from 50 to 200, which may impact performance when rendering multiple curves. Consider making this value configurable based on performance requirements.
- const filterPointsCount = 200; + const filterPointsCount = this._importedSpectrums.getFilterWindowSize() || 200;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
index.html(3 hunks)public/js/webworkers/spectrum-export-worker.js(1 hunks)src/css/main.css(2 hunks)src/graph_imported_curves.js(1 hunks)src/graph_spectrum.js(9 hunks)src/graph_spectrum_calc.js(2 hunks)src/graph_spectrum_plot.js(38 hunks)src/main.js(3 hunks)src/user_settings_dialog.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- index.html
- src/graph_spectrum_calc.js
- src/css/main.css
- src/graph_spectrum.js
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#0
File: :0-0
Timestamp: 2025-06-29T18:40:50.980Z
Learning: In betaflight/blackbox-log-viewer, when implementing PSD heatmap settings persistence in src/graph_spectrum.js, the initialization sequence is critical - user settings must be initialized and applied to GraphSpectrumPlot before attaching input event handlers to prevent spurious refresh() calls that can cause spectrum rendering errors when disabled.
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#833
File: src/graph_spectrum.js:0-0
Timestamp: 2025-05-26T16:18:25.863Z
Learning: In src/graph_spectrum_calc.js, the user created dedicated PSD functions `dataLoadPowerSpectralDensityVsThrottle()` and `dataLoadPowerSpectralDensityVsRpm()` instead of using boolean parameters, following a consistent naming pattern for spectrum data loading methods.
Learnt from: nerdCopter
PR: betaflight/blackbox-log-viewer#822
File: .github/workflows/deploy-preview.yml:17-19
Timestamp: 2025-05-14T16:08:27.254Z
Learning: For betaflight/blackbox-log-viewer repository, the team is aware of the security implications of using `pull_request_target` without specifying `persist-credentials: false` in the checkout action, and has chosen to proceed with the current approach. This concern should not be flagged in future reviews.
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#827
File: src/graph_spectrum.js:200-207
Timestamp: 2025-05-10T19:47:41.430Z
Learning: The vertical zoom slider recalculation in the spectrum analyzer is designed to work only for POWER_SPECTRAL_DENSITY spectrum type, not for PSD_VS_THROTTLE or PSD_VS_RPM types.
public/js/webworkers/spectrum-export-worker.js (4)
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#833
File: src/graph_spectrum.js:0-0
Timestamp: 2025-05-26T16:18:25.863Z
Learning: In src/graph_spectrum_calc.js, the user created dedicated PSD functions `dataLoadPowerSpectralDensityVsThrottle()` and `dataLoadPowerSpectralDensityVsRpm()` instead of using boolean parameters, following a consistent naming pattern for spectrum data loading methods.
Learnt from: nerdCopter
PR: betaflight/blackbox-log-viewer#829
File: src/graph_spectrum_calc.js:332-335
Timestamp: 2025-05-12T19:49:35.667Z
Learning: In the Blackbox Log Viewer, the `_getFlightSamplesFreq()` function should return a sliced array `samples.slice(0, samplesCount)` rather than the full preallocated array, as code that consumes this function expects the array length to match the count of valid samples.
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#0
File: :0-0
Timestamp: 2025-06-29T18:40:50.980Z
Learning: In betaflight/blackbox-log-viewer, when implementing PSD heatmap settings persistence in src/graph_spectrum.js, the initialization sequence is critical - user settings must be initialized and applied to GraphSpectrumPlot before attaching input event handlers to prevent spurious refresh() calls that can cause spectrum rendering errors when disabled.
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#0
File: :0-0
Timestamp: 2025-05-14T08:52:53.032Z
Learning: The FFT.complex library in Betaflight Blackbox Log Viewer provides two main methods: simple(output, input, type) and process(output, outputOffset, outputStride, input, inputOffset, inputStride, type). The 'simple' method is the one currently used in the codebase, while 'process' offers more granular control over memory layout with offsets and strides.
src/user_settings_dialog.js (1)
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#0
File: :0-0
Timestamp: 2025-06-29T18:40:50.980Z
Learning: In betaflight/blackbox-log-viewer, when implementing PSD heatmap settings persistence in src/graph_spectrum.js, the initialization sequence is critical - user settings must be initialized and applied to GraphSpectrumPlot before attaching input event handlers to prevent spurious refresh() calls that can cause spectrum rendering errors when disabled.
src/graph_imported_curves.js (1)
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#833
File: src/graph_spectrum.js:0-0
Timestamp: 2025-05-26T16:18:25.863Z
Learning: In src/graph_spectrum_calc.js, the user created dedicated PSD functions `dataLoadPowerSpectralDensityVsThrottle()` and `dataLoadPowerSpectralDensityVsRpm()` instead of using boolean parameters, following a consistent naming pattern for spectrum data loading methods.
src/graph_spectrum_plot.js (4)
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#833
File: src/graph_spectrum.js:0-0
Timestamp: 2025-05-26T16:18:25.863Z
Learning: In src/graph_spectrum_calc.js, the user created dedicated PSD functions `dataLoadPowerSpectralDensityVsThrottle()` and `dataLoadPowerSpectralDensityVsRpm()` instead of using boolean parameters, following a consistent naming pattern for spectrum data loading methods.
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#0
File: :0-0
Timestamp: 2025-06-29T18:40:50.980Z
Learning: In betaflight/blackbox-log-viewer, when implementing PSD heatmap settings persistence in src/graph_spectrum.js, the initialization sequence is critical - user settings must be initialized and applied to GraphSpectrumPlot before attaching input event handlers to prevent spurious refresh() calls that can cause spectrum rendering errors when disabled.
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#827
File: src/graph_spectrum.js:200-207
Timestamp: 2025-05-10T19:47:41.430Z
Learning: The vertical zoom slider recalculation in the spectrum analyzer is designed to work only for POWER_SPECTRAL_DENSITY spectrum type, not for PSD_VS_THROTTLE or PSD_VS_RPM types.
Learnt from: nerdCopter
PR: betaflight/blackbox-log-viewer#829
File: src/graph_spectrum_calc.js:332-335
Timestamp: 2025-05-12T19:49:35.667Z
Learning: In the Blackbox Log Viewer, the `_getFlightSamplesFreq()` function should return a sliced array `samples.slice(0, samplesCount)` rather than the full preallocated array, as code that consumes this function expects the array length to match the count of valid samples.
src/main.js (3)
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#833
File: src/graph_spectrum.js:0-0
Timestamp: 2025-05-26T16:18:25.863Z
Learning: In src/graph_spectrum_calc.js, the user created dedicated PSD functions `dataLoadPowerSpectralDensityVsThrottle()` and `dataLoadPowerSpectralDensityVsRpm()` instead of using boolean parameters, following a consistent naming pattern for spectrum data loading methods.
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#0
File: :0-0
Timestamp: 2025-06-29T18:40:50.980Z
Learning: In betaflight/blackbox-log-viewer, when implementing PSD heatmap settings persistence in src/graph_spectrum.js, the initialization sequence is critical - user settings must be initialized and applied to GraphSpectrumPlot before attaching input event handlers to prevent spurious refresh() calls that can cause spectrum rendering errors when disabled.
Learnt from: nerdCopter
PR: betaflight/blackbox-log-viewer#829
File: src/graph_spectrum_calc.js:332-335
Timestamp: 2025-05-12T19:49:35.667Z
Learning: In the Blackbox Log Viewer, the `_getFlightSamplesFreq()` function should return a sliced array `samples.slice(0, samplesCount)` rather than the full preallocated array, as code that consumes this function expects the array length to match the count of valid samples.
🔇 Additional comments (14)
public/js/webworkers/spectrum-export-worker.js (3)
4-4: LGTM: Spectrum data length calculation updated.The change from half-length to full-length processing aligns with the broader spectrum data handling improvements in this PR.
7-7: LGTM: Generic CSV headers for broader compatibility.The change from frequency-specific headers ("freq", "value") to generic headers ("x", "y") makes the export format more versatile for different spectrum types.
8-10: LGTM: Loop iteration updated for full data range.The loop now processes all spectrum data points instead of half, which is consistent with the
spectrumDataLengthcalculation change.src/user_settings_dialog.js (2)
234-238: LGTM: New analyser legend settings structure.The new
analyser_legendsettings group follows the established pattern with proper percentage-based positioning and sizing properties.
309-313: LGTM: UI-to-settings conversion properly implemented.The conversion logic correctly reads the input values and formats them as percentage strings, consistent with other UI components.
src/main.js (3)
1152-1157: LGTM: Dynamic filename retrieval with proper error handling.The change to dynamically obtain the export filename from the analyser and the null check with early return provides better error handling and flexibility.
1741-1741: LGTM: Function call updated for new signature.The call to
exportSpectrumToCsv()correctly removes the file parameter argument to match the updated function signature.
1752-1752: LGTM: Method name updated for consistency.The method name change from
clearImportedSpectrums()toremoveImportedSpectrums()provides better semantic clarity.src/graph_imported_curves.js (3)
1-12: LGTM: Well-structured module with clear initialization.The module structure is clean with proper initialization of bounds and clear method definitions.
51-56: LGTM: Proper curve data structure and storage.The curve data structure is well-defined with meaningful properties and proper storage in the internal array.
67-74: LGTM: Clean removal implementation.The
removeCurvesmethod properly clears all data and resets bounds to initial values, ensuring consistent state management.src/graph_spectrum_plot.js (3)
1604-1644: Good implementation of context-aware PSD value display.The code correctly handles two different scenarios:
- For a single PSD spectrum - shows the PSD value at the mouse frequency
- For multiple imported PSD curves - shows the Y-axis value
This provides appropriate user feedback based on the viewing context.
1778-1790: Well-structured import method with appropriate error handling.The method correctly delegates to the appropriate
ImportedCurvesinstance based on spectrum type and provides helpful warnings for unsupported types.Consider returning a boolean to indicate success/failure:
GraphSpectrumPlot.importCurvesFromCSV = function(files) { switch (this._spectrumType) { case SPECTRUM_TYPE.FREQUENCY: this._importedSpectrums.importCurvesFromCSV(files); + return true; break; case SPECTRUM_TYPE.POWER_SPECTRAL_DENSITY: this._importedPSD.importCurvesFromCSV(files); + return true; break; default: console.warn(`Import not supported for spectrum type: ${this._spectrumType}`); + return false; break; } };
433-461: No action needed: ignore undefineduserSettingsand encapsulation flags.The method’s use of
userSettingsis consistent with the rest of the codebase—globalThis.userSettingsis initialized insrc/main.jsand treated as a global throughout all graph modules. Likewise,importedCurves[row].nameaccesses a public property; there’s no_curvesDataor private‐field breach.Likely an incorrect or invalid review comment.
| this.importCurvesFromCSV = function(files) { | ||
| let importsLeft = maxImportCount - this._curvesData.length; | ||
|
|
||
| for (const file of files) { | ||
| if (importsLeft-- == 0) { | ||
| break; | ||
| } | ||
| const reader = new FileReader(); | ||
| reader.onload = function (e) { | ||
| try { | ||
| const stringRows = e.target.result.split("\n"); | ||
|
|
||
| const header = stringRows[0].split(","); | ||
| if (header.length != 2 || header[0] != "x" || header[1] != "y") { | ||
| throw new SyntaxError("Wrong curves CSV data format"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance CSV validation and error handling.
The CSV parsing logic has several areas for improvement:
- The header validation could be more flexible
- Missing validation for numeric values
- Consider trimming whitespace from CSV values
const header = stringRows[0].split(",");
-if (header.length != 2 || header[0] != "x" || header[1] != "y") {
+if (header.length != 2 || header[0].trim() != "x" || header[1].trim() != "y") {
throw new SyntaxError("Wrong curves CSV data format");
}Additionally, consider adding validation for the parsed numeric values:
const curvesData = stringRows.map( function(row) {
const data = row.split(","),
- x = parseFloat(data[0]),
- y = parseFloat(data[1]);
+ x = parseFloat(data[0].trim()),
+ y = parseFloat(data[1].trim());
+ if (isNaN(x) || isNaN(y)) {
+ throw new SyntaxError(`Invalid numeric values in CSV: ${row}`);
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/graph_imported_curves.js around lines 14 to 29, improve the CSV
validation by making the header check more flexible to allow for whitespace and
case variations, trim whitespace from all CSV values before processing, and add
validation to ensure that the parsed x and y values are numeric. Update the code
to trim each header and data value, verify the header columns
case-insensitively, and check that each x and y value can be converted to a
valid number, throwing an error if not.
|
|
||
| canvasCtx.beginPath(); | ||
| canvasCtx.lineWidth = 1; | ||
| canvasCtx.strokeStyle = curvesColors[spectrumNum]; | ||
| canvasCtx.strokeStyle = this.curvesColors[spectrumNum]; |
There was a problem hiding this comment.
Prevent undefined colors for curves beyond the color array length.
Direct array access will return undefined for indices >= 5, potentially causing rendering issues.
Use a modulo operation or the suggested getCurveColor method:
- canvasCtx.strokeStyle = this.curvesColors[spectrumNum];
+ canvasCtx.strokeStyle = this.curvesColors[spectrumNum % this.curvesColors.length];Also applies to: 375-375, 456-456
🤖 Prompt for AI Agents
In src/graph_spectrum_plot.js at line 272 (and similarly at lines 375 and 456),
the code directly accesses this.curvesColors with spectrumNum, which can be out
of bounds and return undefined. To fix this, replace direct array access with a
modulo operation on spectrumNum against the length of this.curvesColors or use
the existing getCurveColor method to safely retrieve a valid color, ensuring no
undefined colors are used for rendering.
| const spectrumCount = this._importedSpectrums.curvesCount(); | ||
| for (let spectrumNum = 0; spectrumNum < spectrumCount; spectrumNum++) { | ||
| const curvesPonts = this._importedSpectrumsData[spectrumNum].points; | ||
| const curvesPonts = this._importedSpectrums._curvesData[spectrumNum].points; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use consistent API for accessing ImportedCurves data.
The code inconsistently uses curvesCount() method but directly accesses the private _curvesData property. This violates encapsulation principles.
The ImportedCurves class should provide public methods to access curve data:
- const spectrumCount = this._importedSpectrums.curvesCount();
- for (let spectrumNum = 0; spectrumNum < spectrumCount; spectrumNum++) {
- const curvesPonts = this._importedSpectrums._curvesData[spectrumNum].points;
+ const spectrumCount = this._importedSpectrums.curvesCount();
+ for (let spectrumNum = 0; spectrumNum < spectrumCount; spectrumNum++) {
+ const curvesPonts = this._importedSpectrums.getCurvePoints(spectrumNum);Also applies to: 369-371
🤖 Prompt for AI Agents
In src/graph_spectrum_plot.js around lines 265 to 267 and also lines 369 to 371,
the code uses the public method curvesCount() but directly accesses the private
property _curvesData, breaking encapsulation. To fix this, replace direct
accesses to _curvesData with appropriate public getter methods provided by the
ImportedCurves class to retrieve curve data, ensuring all interactions with
curve data go through the class's public API.
demvlad
changed the title
|
@coderabbitai thank's for all notices. |
|
Tip For best results, initiate chat on the files or code changes. @demvlad You're very welcome! It's great to see this PSD segment length improvement come together so well. The refactoring looks solid, and the new numeric input approach for controlling the Welch method parameters is much more intuitive than the previous slider. The modularization of the imported curves handling and the filter alignment fixes are excellent improvements too. Best of luck with the final stages of this PR! 🐰 🧠 Learnings used |
The vertical slider is changed to numerical input fields.
The older vertical slider had different function by compare with other spectrums, what was unlogical.
This settings shows points per segment for PSD computing by using Welch method.
This value is power at 2. The bigger segment length gives bigger spectrum frequency resolution, but the less segments length gives smoother result. The maximal value is limited by data samples count.
The new number input field:
The olde vertical slider:
Summary by CodeRabbit
New Features
Style
Bug Fixes